Fix up the aws-lc-verification and blst-verification CI runs #2199
Conversation
s2nTests/docker/blst.dockerfile
Outdated
| # XXX: except right now it's the head of a topic branch with some | ||
| # changes to fix some problems with this test run. If it all works, | ||
| # that branch should get merged and this commit hash should be updated | ||
| # again. |
There was a problem hiding this comment.
Make sure to update this comment before landing.
There was a problem hiding this comment.
My assumption here is that the redirected repo fetches are for branches and you are establishing a base for changes due to core saw-script changes. I don't have full details, but in talking with members of the team doing these verifications originally, there was specific intent to target the fork/branch as opposed to following upstream, so I'm just passing along a caution that upstream might be divergent in unknown ways.
Other than the above concern and the suggestion regarding the env variable name, this looks reasonable to me.
| @@ -1,9 +1,11 @@ | |||
| #!/usr/bin/env bash | |||
| set -xe | |||
|
|
|||
| export IN_SAW_CI=yes | |||
There was a problem hiding this comment.
I would suggest using the CI_TEST_LEVEL variable that we use in other repos for consistency. It has apparently not been set yet in the CI configuration for saw or cryptol, but it is used in numerous other repositories (e.g. https://github.com/GaloisInc/crucible/blob/master/.github/workflows/crux-llvm-build.yml#L62 and https://github.com/GaloisInc/what4/blob/038e948817b092ac7750e073f1d430ee293f3c20/.github/workflows/test.yml#L61).
There was a problem hiding this comment.
So the reason I didn't do that, or the reason not to do that, or something, is that it should be a variable that's specific to SAW (and, ideally, these particular test runs) and that one isn't. In particular using it for this purpose would prevent using it in either of the other trees' CI logic.
...and given that one of the blst-verification CI jobs seems to run for >6 hours, if that tree ever comes back to life they probably should use it. :-|
Right, both new commits are small changes on top of the previous version we were using. In the case of blst-verification that's main; in the case of aws-lc-verification it's almost certainly a can of worms. |
This will keep their docker containers from downloading SAW and Cryptol bins that we aren't going to use. Don't croak removing bin/saw if it's not already there.
Add some basic commentary about what they point to.
sauclovian-g
force-pushed
the
2166-fix-ci
branch
from
d808fe6 to
8f2d3b6
Compare
|
(I force-pushed to squash the noise and the multiple intermediate blst-verification commit hashes, none of which are valid any more after merging over there because of the repo config.) |
Refer to new commits in both repositories that both respond to
$IN_SAW_CIto avoid downloading and also, when downloading, don't try to fetch ancient nightlies that have disappeared.Closes #2166.